fix(discovery): exhaust paginated tools/list before comparison - #633
Conversation
Signed-off-by: Noah Ingwers <98993329+noah-ing@users.noreply.github.com>
|
🟡 Contributor Check: MEDIUM
Automated check by AgenTrust Contributor Check. |
github-actions
Bot
added
the
needs-review:MEDIUM
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: Noah Ingwers <98993329+noah-ing@users.noreply.github.com>
Signed-off-by: Noah Ingwers <98993329+noah-ing@users.noreply.github.com>
|
Reviewed the pagination exhaustion logic in discovery.py and its integration into proxy.py and stdio.py, cursor handling including the empty string vs absent distinction, duplicate and cycle detection, the page budget boundary, and stdio cancellation cleanup. All of that checks out and the 79 new tests cover it well. One thing worth flagging, not blocking, more a heads up on cost. _check_upstream_drift and _check_provenance each call _advertised_tools independently, with separate first contact caches, self._drift_checked and self._provenance. On the very first tool call against a server that has provenance_record_path configured, both caches are empty, so _call_tool_impl calls _check_upstream_drift directly, then later in the same call _forward_to_upstream calls _check_provenance, and both trigger the full paginated tools/list fetch. Before this PR that was cheap either way, one request per call. Now _advertised_tools can be up to MAX_DISCOVERY_PAGES round trips, and cold start against a server with provenance configured just paid that cost twice back to back instead of once, doubling stdio child process round trips too on that path. Given the PR's own note that the page limit bounds request count rather than elapsed time, this seems worth closing, maybe by caching the _advertised_tools() result itself per server per session rather than caching only the two downstream comparisons, so both callers share one fetch. Happy to see this land separately if you would rather not hold the PR on it. |
Signed-off-by: Noah Ingwers <98993329+noah-ing@users.noreply.github.com>
|
Thanks for tracing the cold-call path — you're right. Fixed in 8b8ba3b. Drift and provenance now share one completed paginated acquisition per server/publisher authority per session. Concurrent readers wait for that acquisition, and a drift check is not marked complete while discovery is still in flight. Completed unchecked outcomes are cached distinctly from empty catalogs; cancellation is not cached. Session rebinding resets the acquisition and both comparison caches together, with old in-flight results discarded before reuse. Added 25 regressions. The 12 public-call transport cases use HTTP JSON/SSE or real stdio subprocess I/O and actual signed TRACE provenance verification. All 12 fail on the preceding commit with four discovery requests instead of two, then pass with this change. They also cover warm reuse, warn-only mismatch, and incomplete discovery with and without a required-provenance floor. The other cases cover concurrency, cancellation ownership, identity isolation, and session reset. Full host and clean no-cache Linux runs each pass 1,803 tests, with six unchanged optional/hardware skips. Ruff, mypy, Bandit, dependency/secret scans, and fresh wheel/sdist installation checks pass; all 29 newly changed executable lines are covered. The existing unchecked-call policy is unchanged, as are the page-count versus elapsed-time limit and non-atomic-snapshot caveats. The changelog credits your observation. |
carloshvp
left a comment
carloshvp
left a comment
There was a problem hiding this comment.
Reviewed exact head 8b8ba3b. No blocking findings in the pagination and discovery-cache change.
Complete acquisition precedes drift/provenance comparison on HTTP JSON/SSE and stdio. Later-page failures, invalid envelopes, duplicate names, cursor cycles, and over-budget continuations do not yield a partial catalog. Concurrent checks share completed acquisition; cancellation is not cached, and rebinding discards old acquisition/cache state. The existing unchecked-discovery policy remains separate from the configured required-provenance floor.
Independent isolated macOS/Python 3.13 validation: full suite 1,845 passed, 14 skipped; Ruff, mypy, Bandit and whitespace checks passed. An additional seeded matrix exercised 300 multi-page acquisitions, including 100 late duplicate failures, plus the actual 1,000-page completion/continuation boundary. Replacing the changed proxy/stdio implementations with main's versions makes the relevant transport suites fail: 62 failed, 29 passed. Current tests include signed TRACE provenance and real stdio subprocess transport, not only asserted verification results.
The merge simulation against freshly fetched main is conflict-free and all four commits have DCO sign-offs. #634 touches the same session/resource lifecycle code and will need careful integration and fresh combined validation when the second PR lands. This approval covers this head independently; remaining repository gates and platform/hardware skips are not cleared by local testing.
Preserve the admission, drain, and retryable cleanup semantics from agentrust-io#634 while invalidating shared discovery and comparison caches together. Add public-call pagination/drain regressions across HTTP JSON, SSE, and stdio, plus failed-cleanup cache tests. Signed-off-by: Noah Ingwers <98993329+noah-ing@users.noreply.github.com>
Signed-off-by: Noah Ingwers <98993329+noah-ing@users.noreply.github.com>
carloshvp
left a comment
carloshvp
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 1a01914 after integration with merged #634. No blocking findings.
The lifecycle integration preserves admission/drain/retry behavior while invalidating the advertised catalog, discovery locks, provenance, and drift caches together before resource cleanup. The new public-call tests exercise normal drain and deadline cancellation through HTTP JSON, SSE, and real stdio with signed TRACE provenance. The Windows socket-reset adjustment is confined to expected teardown errors in the test gate.
Independent isolated macOS/Python 3.13 validation: full suite 1,887 passed, 14 skipped; Ruff, mypy, Bandit and whitespace checks passed. Three additional real-subprocess endpoint checks verify that close, reset and shutdown clear the paginated acquisition cache and locks, terminate the old child, and (for close/reset) reacquire both pages in a fresh successor. Those checks were mutation-tested against the earlier cleanup behavior: all three fail if cleanup clears only provenance/drift and retains the new acquisition caches.
I had prepared a local conflict resolution, but the final pre-push check found your newer commits. I did not push over them. Their runtime integration matches the independently validated fix; the results above are from your exact new head. Current hosted technical checks pass, the fetched-main merge simulation is clean, and the commits are signed off. Approving this head; hardware/platform skips remain outside local validation.
imran-siddique
left a comment
imran-siddique
left a comment
There was a problem hiding this comment.
Approving. Read the acquisition loop rather than the summary, because the property #631 is about is the one that is easy to implement almost-correctly.
It terminates on "nextCursor" not in result alone, so an empty-string cursor continues the listing. That is the case the MCP spec makes explicit and the one a truthiness check gets wrong, and it is the difference between exhausting a catalog and stopping one page early on a server that pages with "".
The discard paths are the other half and they are all present: a malformed envelope, a malformed page, a non-string or empty tool name, a duplicate tool name, a non-string cursor, a repeated cursor, and the page ceiling each raise rather than return what was collected so far. A partial acquisition never reaches the drift or provenance comparison, which is what #631 was actually about: the comparison was being fed a truncated catalog and reporting agreement.
141 passed and 1 skipped across the six discovery test files here.
Merging.
5 participants
What
Exhaust upstream
tools/listpagination before drift or provenance comparison, using one acquisition helper for HTTP and stdio.nextCursorcompletes discovery.Why
Closes #631. Credit to @solloek369-arch for the observation on #566 and @imran-siddique for confirming and filing the issue.
An approved tool on page two previously looked withdrawn; conversely, a matching first page could conceal failed acquisition of a later page. The initial transport regressions on unchanged production code produced 37 failures / 29 existing passes.
The duplicate-acquisition cost identified by qubeena07 is addressed in
8b8ba3b. Twelve additional public-call transport regressions fail on the preceding commit with four discovery requests instead of two, then pass with the shared cache. Both comparisons still run against the completed listing.Security impact
Incomplete acquisition is
unchecked, never a comparison against partial tools. The existing behavior allowing calls when discovery is unchecked remains unchanged; this PR does not turn that state into a fail-closed authorization gate. A configured required-provenance floor still prevents forwarding when provenance is unchecked.No changes to approval semantics, catalog hash construction, checkpoint binding, signature behavior, or #566 acceptance-matrix redesign. The page limit bounds request count, not total elapsed time or bytes. Pagination does not provide an atomic snapshot of a changing server. Validation covers acquisition shapes and unambiguous names, not the full MCP tool schema. First-contact caching is not continuous monitoring: completed unchecked acquisitions are cached too. Cancellation is not cached, but the existing stdio child-close behavior does not automatically restart that child. These limits are recorded in
LIMITATIONS.md.Integration with #634
Merged current
main(c9d0133) into this branch ata9be23a. The lifecycle's admission/drain ordering and retryable resource cleanup are preserved. Cleanup invalidates all four discovery/comparison cache containers before any cleanup await or empty-resource return; a failed child close does not adopt the successor.Eight additional cases cover normal drain and owner/waiter cancellation across HTTP JSON, SSE, and real stdio, plus empty/failed cleanup. They assert old-call finalization before rebind, fresh successor resources and discovery, signed provenance, and separated audit records. The six transport/lifecycle cases also passed ten consecutive repetitions. The defensive cache-generation unit test is explicitly separate from the public lifecycle: rebinding while public calls remain active is not permitted.
Windows CI on
a9be23afound a test-teardown socket reset after the stdio child was deliberately terminated.1a01914narrowly handles that expected reset atwait_closed(); the authorization, ordering, cancellation, and resource assertions are unchanged. Full host and clean no-cache container checks were rerun on this final head. Final hosted CI passed all six Python 3.11–3.13 / Ubuntu and Windows test jobs plus governance. CodeQL, changed-code fuzzing, Docker, and limitations parity passed; the conditional benchmark was skipped. Current-head maintainer approval remains required.Test plan — integrated head
1a01914pip check; runtime network disabled, non-root, all capabilities dropped.ruff check src/ tests/andmypy src/cmcp_runtime/ src/cmcp_verify/pass. Ruff formatting passes for the new test files and modified production-code range; unrelated pre-existing formatting is unchanged.VERIFIED, complete change →CATALOG_MISMATCH, incomplete discovery →UNCHECKED.--skip-editable), Gitleaks diff scan, detect-secrets changed-file scan, and whitespace checks pass.Host and container use the repository's hash-locked
requirements/dev.txt; no dependencies were added or changed. These are software protocol regressions, not hardware-attestation evidence.DCO sign-off